Conversation
…e runners for the matrix
|
pre-commit.ci autofix |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
The alignment transformation is a concern here, there's a few other smaller things.
src/openfe_analysis/rmsd.py
Outdated
| ) -> tuple[mda.core.groups.AtomGroup, list[mda.core.groups.AtomGroup]]: | ||
| protein = u.select_atoms(protein_selection) | ||
| lig_atoms = u.select_atoms(ligand_selection) | ||
| # split into individual ligands by residue |
There was a problem hiding this comment.
This is dangerous, you can't guarantee that a ligand will always be a single residue.
There was a problem hiding this comment.
It would be probably safer to select by fragments - guessing bonds if necessary.
There was a problem hiding this comment.
Changed this!
| ligand_selection: str = "resname UNK", | ||
| protein_selection: str = "protein and name CA", |
There was a problem hiding this comment.
This is fine for now, but in the refactor just take in atomgroups since we can't guarantee residue / atom names going forward, especially with rosemary.
| # - make the ligands not jump periodic images between frames | ||
| # - align the ligands to minimise its RMSD | ||
| for lig in ligands: | ||
| u.trajectory.add_transformations(NoJump(lig), Aligner(lig)) |
There was a problem hiding this comment.
This would align the trajectory to minimize ligand 1 and then re-align to ligand 2. I'm not sure it's going to be done this corrrectly - would it not effectively mess up the RMSD for the first ligand?
| return u | ||
|
|
||
|
|
||
| def twoD_RMSD(positions: np.ndarray, w: Optional[npt.NDArray]) -> list[float]: |
There was a problem hiding this comment.
This is fine, but please open an issue to switch this to using something better, like this: https://userguide.mdanalysis.org/1.1.1/examples/analysis/alignment_and_rms/pairwise_rmsd.html#Pairwise-RMSD-of-a-trajectory-to-itself
src/openfe_analysis/rmsd.py
Outdated
| ligand_com_drift : list of list[float] or None | ||
| COM drift of each ligand per frame. | ||
| """ | ||
| traj_slice = u.trajectory[::skip] |
There was a problem hiding this comment.
This is probably dangerous to do in terms of the MDAnalysis internals (it's not a pure Python object). It is better if you enumerate over u.trajectory[::skip] instead on line 183.
There was a problem hiding this comment.
Changed this!
| "protein_RMSD": [], | ||
| "ligand_RMSD": [], | ||
| "ligand_wander": [], | ||
| "ligand_COM_drift": [], |
There was a problem hiding this comment.
Note - this will be a breaking change downstream, please don't put this in the next release.
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
+ Coverage 88.16% 95.94% +7.77%
==========================================
Files 7 6 -1
Lines 338 345 +7
==========================================
+ Hits 298 331 +33
+ Misses 40 14 -26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Note: DNM since this will break analysis in openfe protocols
Renamed
ligand_wandertoligand_COM_driftligand_rmsdandligand_COM_driftare now more nested, storing information for multiple ligands if presentExpose selection indices for protein and ligands
Enable ligand RMSD and wander calculation for files containing more than one ligands (e.g. SepTop). Ligands can be selected based on a MDAnalysis selection string. This assumes ligands are in separate residues.